Skip to content

feat(client): impl Debug for Client#430

Open
zachschuermann wants to merge 4 commits into
ClickHouse:mainfrom
zachschuermann:client-debug
Open

feat(client): impl Debug for Client#430
zachschuermann wants to merge 4 commits into
ClickHouse:mainfrom
zachschuermann:client-debug

Conversation

@zachschuermann
Copy link
Copy Markdown

@zachschuermann zachschuermann commented May 20, 2026 •
edited
Loading

Summary

Implement Debug for Client.

Since Client has both sensitive information and some non-debugable fields we manually impl Debug by:

  1. #[derive(Debug)] on ProductInfo
  2. removing http client and insert_metadata_cache
  3. redacting authorization, headers values, and only printing url.origin to avoid leaking user/pass

Fixes #429

Checklist

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided so that we can include it in CHANGELOG later

Copilot AI review requested due to automatic review settings May 20, 2026 22:10
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 20, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI reviewed May 20, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements a manual Debug for Client to unblock downstream #[derive(Debug)] use cases while avoiding leaking sensitive client configuration.

Changes:

  • Added #[derive(Debug)] to ProductInfo to support debug output.
  • Implemented a manual Debug for Client that omits non-debuggable/internal fields and redacts auth/header values.
  • Added unit tests to validate debug formatting and redaction behavior.
Comments suppressed due to low confidence (1)

src/lib.rs:134

  • When the test-util feature is enabled, Client has a mocked: bool field that affects behavior (see with_url handling of mocked URLs). The manual Debug impl currently hides it, which makes debugging tests harder. Consider including mocked behind the same cfg in the Debug output (it’s not sensitive and is trivially debuggable).
            .field("compression", &self.compression)
            .field("roles", &self.roles)
            .field("settings", &self.settings)
            .field("headers", &self.headers.keys())
            .field("products_info", &self.products_info)
            .field("validation", &self.validation)
            .finish_non_exhaustive()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/lib.rs
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs
abonander
abonander requested changes May 21, 2026
View reviewed changes
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs
Comment thread src/lib.rs
@zachschuermann zachschuermann requested a review from abonander May 21, 2026 18:00
zachschuermann
zachschuermann commented May 21, 2026
View reviewed changes
Comment thread src/lib.rs
@zachschuermann
Copy link
Copy Markdown
Author

zachschuermann commented May 27, 2026

hi @abonander friendly ping on this!

abonander
abonander approved these changes May 28, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll end up improving this over time but since Debug doesn't need to be stable, having something is better than nothing.

@abonander
Copy link
Copy Markdown
Contributor

abonander commented May 28, 2026

@zachschuermann looks like you still need to sign the CLA (see above). And rebase as well, please.

@abonander
Copy link
Copy Markdown
Contributor

abonander commented May 28, 2026

This is ready to merge otherwise, but it still says you haven't signed the CLA. Did you do it and it just hasn't updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@abonander abonander abonander approved these changes

@kavirajk kavirajk Awaiting requested review from kavirajk kavirajk is a code owner

@joe-clickhouse joe-clickhouse Awaiting requested review from joe-clickhouse joe-clickhouse is a code owner

@peter-leonov-ch peter-leonov-ch Awaiting requested review from peter-leonov-ch peter-leonov-ch is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

implement Debug for Client

4 participants

@zachschuermann @CLAassistant @abonander